-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Eligibility API logic #286
Conversation
@@ -0,0 +1,7 @@ | |||
import Foundation | |||
|
|||
enum SupportedPaymentMethodsType: String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this enum based on the logic that was previously removed:
paypal-ios/Sources/CorePayments/Networking/GraphQL/SupportedPaymentMethodsType.swift
Line 1 in 257df31
enum SupportedPaymentMethodsType: String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
let paypal: SupportedPaymentMethodsTypeEligibility | ||
let paylater: SupportedPaymentMethodsTypeEligibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let paypal: SupportedPaymentMethodsTypeEligibility | |
let paylater: SupportedPaymentMethodsTypeEligibility | |
let payPal: SupportedPaymentMethodsTypeEligibility | |
let payLater: SupportedPaymentMethodsTypeEligibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated: cd016e3
} | ||
do { | ||
let config = try await configManager.getCoreConfig() | ||
let eligibilityRequest = EligibilityRequest(currencyCode: "USD", intent: .CAPTURE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take it or leave it - maybe we want to leave a TODO for this, but this intent will have to be the same intent used to create the orderID which will eventually get used when constructing a VenmoCheckoutRequest(orderID: "<ORDER_ID>")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
extension EligibilityAPI { | ||
|
||
static let rawQuery = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we really need this to be in an extension here. Was there a particular reason for using it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to put the query in an extension to make the code more readable. I based it on this implementation where the query is in the same method, which makes the method much larger. Basically, it's to separate(ish) responsibilities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also throw it in a private method under check
if we want to separate it but move it out of the extension
@@ -1,9 +1,36 @@ | |||
import Foundation | |||
|
|||
/// The `EligibilityClient` class provides methods to check eligibility status based on provided requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 Nice docstrings
@@ -0,0 +1,10 @@ | |||
import Foundation | |||
|
|||
public enum EligibilityIntent: String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I wonder if we only support CAPTURE
and AUTHORIZE
. Per these PP docs, these are the only 2 you can create an orderID with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be OK to just start with those 2. Also needs a docstring since public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 docs says SALE
and AUTHORIZE
, thanks for your feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, I also changed cases to lowercase to be more Swifty
: 54e53c0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very clean docstrings - lgtm!! 🚀
|
||
let configManager = CoreConfigManager(domain: "Venmo Payments") | ||
|
||
@Published var state = VenmoState() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take it our leave it for this PR, but we started a transition to use a new CurrentState
enum across all of the views vs individualized views with custom states. If you take a look at the PayPal Web Payments views you can see where that was in motion. Not a blocker, would just be nice to eventually share duplicated states with a shared interface. Can totally be a follow up or something we tackle when we address the other view since only PayPal Web was updated thus far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, thanks for your feedback: 33d3d46
PayPal.xcodeproj/project.pbxproj
Outdated
@@ -570,31 +581,27 @@ | |||
path = Models; | |||
sourceTree = "<group>"; | |||
}; | |||
62D3FB122C3DB4B30046563B /* VenmoPaymentsTests */ = { | |||
459633142C46BD51002008EF /* Recovered References */ = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing we can remove these Recovered References
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, updated: f9f2df3
|
||
extension EligibilityAPI { | ||
|
||
static let rawQuery = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also throw it in a private method under check
if we want to separate it but move it out of the extension
} | ||
|
||
var asResult: EligibilityResult { | ||
.init( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.init( | |
EligibilityResult( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 644df70
@@ -0,0 +1,7 @@ | |||
import Foundation | |||
|
|||
enum SupportedPaymentMethodsType: String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to lowercase these as well or keep as is since it's internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea!!! Done: b422115
Co-authored-by: Jax DesMarais-Leder <[email protected]>
Reason for changes
DTMOBILES-777
Summary of changes
Checklist
Authors